fix: dedup commutative AND/OR operands when canonicalize is disabled#23615
Open
1fanwang wants to merge 5 commits into
Open
fix: dedup commutative AND/OR operands when canonicalize is disabled#236151fanwang wants to merge 5 commits into
1fanwang wants to merge 5 commits into
Conversation
`SimplifyExpressions` disables canonicalization for `LogicalPlan::Join` (see apache#8780), so `expr_contains_inner`'s structural `==` cannot recognize duplicate predicates that differ only by operand order such as `A = B` and `B = A`. Workloads that simplify expressions inside a join filter -- notably the delta-rs MERGE case reported in apache#14943 -- end up retaining the duplicate even after multiple simplifier cycles. Switching the leaf comparison to `NormalizeEq` (the same trait CSE uses to recognize common sub-expressions for `+`, `*`, `&`, `|`, `^`, `=`, `!=`) makes the dedup commutativity-aware while falling back to structural `==` for everything else, so existing volatility guards and non-commutative rules are unchanged. Closes apache#14943. Signed-off-by: 1fanwang <1fannnw@gmail.com>
alamb
approved these changes
Jul 15, 2026
| physical_plan | ||
| 01)ProjectionExec: expr=[column1@0 = 1 as opt1, column1@0 = 2 AND column1@0 != 2 as noopt1, column1@0 = 4 as opt2, column1@0 != 5 AND column1@0 = 5 as noopt2] | ||
| 02)--DataSourceExec: partitions=1, partition_sizes=[1] | ||
|
|
Contributor
There was a problem hiding this comment.
I took the liberty of adding some slt coverage for this feature too
Contributor
There was a problem hiding this comment.
These fail like this on main
warning: `datafusion-sqllogictest` (test "sqllogictests") generated 1 warning
Finished `ci` profile [unoptimized] target(s) in 1m 45s
Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-01bd002bb437dcb6)
Running with 16 test threads (available parallelism: 16)
Completed 493 test files in 6 seconds External error: 2 errors in file /Users/andrewlamb/Software/datafusion/datafusion/sqllogictest/test_files/simplify_expr.slt
1. query result mismatch:
[SQL] explain select * from t1 join t2 on t1.a = t2.b or t2.b = t1.a;
[Diff] (-expected|+actual)
logical_plan
- 01)Inner Join: t1.a = t2.b
+ 01)Inner Join: Filter: t1.a = t2.b OR t2.b = t1.a
02)--TableScan: t1 projection=[a]
03)--TableScan: t2 projection=[b]
physical_plan
- 01)HashJoinExec: mode=CollectLeft, join_type=Inner, on=[(a@0, b@0)]
+ 01)NestedLoopJoinExec: join_type=Inner, filter=a@0 = b@1 OR b@1 = a@0
02)--DataSourceExec: partitions=1, partition_sizes=[1]
03)--DataSourceExec: partitions=1, partition_sizes=[1]
at /Users/andrewlamb/Software/datafusion/datafusion/sqllogictest/test_files/simplify_expr.slt:162
2. query result mismatch:
[SQL] explain select * from t1 join t2 on t1.a > t2.b and (t1.a + t2.b > 1 or t2.b + t1.a > 1);
[Diff] (-expected|+actual)
logical_plan
- 01)Inner Join: Filter: t1.a > t2.b AND t1.a + t2.b > Int32(1)
+ 01)Inner Join: Filter: t1.a > t2.b AND (t1.a + t2.b > Int32(1) OR t2.b + t1.a > Int32(1))
02)--TableScan: t1 projection=[a]
03)--TableScan: t2 projection=[b]
physical_plan
- 01)NestedLoopJoinExec: join_type=Inner, filter=a@0 > b@1 AND a@0 + b@1 > 1
+ 01)NestedLoopJoinExec: join_type=Inner, filter=a@0 > b@1 AND (a@0 + b@1 > 1 OR b@1 + a@0 > 1)
02)--DataSourceExec: partitions=1, partition_sizes=[1]
03)--DataSourceExec: partitions=1, partition_sizes=[1]
at /Users/andrewlamb/Software/datafusion/datafusion/sqllogictest/test_files/simplify_expr.slt:181
Author
|
Thanks a lot @alamb checking now |
alamb
enabled auto-merge
July 16, 2026 18:58
alamb
disabled auto-merge
July 16, 2026 18:58
Contributor
|
🤔 there appears to be some sort of CI failure |
`NormalizeEq for Expr` compared `ScalarFunction`, `AggregateFunction`, and `WindowFunction` by `func.name()` only, so two distinct functions sharing a display name (as programmatically-built plans like delta-rs MERGE produce) were treated as semantically equal. Once the simplifier's AND/OR dedup routes through `normalize_eq`, this collapsed `regex_udf(x) AND regex_udf(x)` -- two different regex UDFs that share the name -- into a single predicate, changing query results and failing `test_parameterized_scalar_udf`. Compare the full function (as structural `==` already does) so `normalize_eq` adds only commutative-operand awareness on top of equality. Registry-parsed queries reuse one instance per name, so real SQL is unchanged; CSE simply stops merging genuinely distinct same-named functions. Signed-off-by: 1fanwang <1fannnw@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Revives #21870, which was closed by the stale bot. The direction was agreed with
@asolimandoandsql_plannerwas benchmarked by@alambthere (results, no measurable change).Rationale for this change
SimplifyExpressionsdisables canonicalization forLogicalPlan::Join(see #8780), so the AND/OR dedup inexpr_contains_inner— which compares each conjunct/disjunct leaf with structural==— cannot recognize duplicates that differ only by commutative operand order (A = BvsB = A). Simplifying inside a join filter (the delta-rs MERGE case reported in #14943) keeps the duplicate across simplifier cycles because nothing normalizes operand order first.@alambpointed at the fix in the issue thread: CSE already dedupsA = B/B = AviaNormalizeEq; route the simplifier's leaf comparison through the same trait.What changes are included in this PR?
expr_contains_innercompares leaves withExpr::normalize_eqinstead of==.NormalizeEqhandles+,*,&,|,^,=,!=commutatively and falls back to structural==for everything else, so non-commutative rules and the existing!needle.is_volatile()guard are unchanged. A regression test covers theAND,OR, and 3-conjunct nested forms.NormalizeEq for Expralso now compares scalar/aggregate/window functions by full identity rather thanname()alone. It previously treated two distinct functions that share a display name as equal, so routing the dedup through it collapsedregex_udf(x) AND regex_udf(x)(two different UDFs, same name) into one predicate. Registry-parsed SQL reuses one instance per name and is unaffected; this only stops merging genuinely distinct same-named functions, which programmatically-built plans (delta-rs MERGE, the #14943 reporter) can produce.Are these changes tested?
Yes. The new
test_simplify_swapped_operands_in_and_or_no_canonicalizefails onmain(the duplicate passes through unchanged) and passes with the fix, and@alamb'ssimplify_expr.sltcases cover the join path. The existingtest_parameterized_scalar_udfguards the function-identity fix.datafusion-optimizer(753),datafusion-common(509),datafusion-expr(235), CSE, and the UDF integration suite pass;cargo fmt/clippyclean;sql_plannershows no measurable change.Are there any user-facing changes?
No public API changes. AND/OR chains containing commutative-equivalent duplicates now collapse even when the simplifier's canonicalizer is disabled (currently the
LogicalPlan::Joinpath). Canonicalize-on paths produce the same output as before.